Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[generator_interior] Be more precise with scopes of borrowed places #94309

Merged
merged 8 commits into from
Mar 18, 2022

Conversation

eholk
Copy link
Contributor

@eholk eholk commented Feb 23, 2022

Previously the generator interior type checking analysis would use the nearest temporary scope as the scope of a borrowed value. This ends up being overly broad for cases such as:

fn status(_client_status: &Client) -> i16 {
    200
}

fn main() {
    let client = Client;
    let g = move || match status(&client) {
        _status => yield,
    };
    assert_send(g);
}

In this case, the borrow &client could be considered in scope for the entirety of the match expression, meaning it would be viewed as live across the yield, therefore making the generator not Send.

In most cases, we want to use the enclosing expression as the scope for a borrowed value which will be less than or equal to the nearest temporary scope. This PR changes the analysis to use the enclosing expression as the scope for most borrows, with the exception of borrowed RValues which are true temporary values that should have the temporary scope. There's one further exception where borrows of a copy such as happens in autoref cases also should be ignored despite being RValues.

Joint work with @nikomatsakis

Fixes #57017

r? @tmandry

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 23, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 23, 2022
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Feb 25, 2022

☔ The latest upstream changes (presumably #94350) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Mar 5, 2022

☔ The latest upstream changes (presumably #94634) made this pull request unmergeable. Please resolve the merge conflicts.

eholk and others added 2 commits March 10, 2022 17:14
Also includes a lengthy comment arguing the correctness.

Co-authored-by: Niko Matsakis <[email protected]>
@tmandry
Copy link
Member

tmandry commented Mar 17, 2022

The change makes sense to me and is well documented. Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Mar 17, 2022

📌 Commit 2fcd542 has been approved by tmandry

@bors
Copy link
Contributor

bors commented Mar 17, 2022

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 17, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 18, 2022
Rollup of 10 pull requests

Successful merges:

 - rust-lang#91133 (Improve `unsafe` diagnostic)
 - rust-lang#93222 (Make ErrorReported impossible to construct outside `rustc_errors`)
 - rust-lang#93745 (Stabilize ADX target feature)
 - rust-lang#94309 ([generator_interior] Be more precise with scopes of borrowed places)
 - rust-lang#94698 (Remove redundant code from copy-suggestions)
 - rust-lang#94731 (Suggest adding `{ .. }` around a const function call with arguments)
 - rust-lang#94960 (Fix many spelling mistakes)
 - rust-lang#94982 (Add deprecated_safe feature gate and attribute, cc rust-lang#94978)
 - rust-lang#94997 (debuginfo: Fix ICE when generating name for type that produces a layout error.)
 - rust-lang#95000 (Fixed wrong type name in comment)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8499a8b into rust-lang:master Mar 18, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 18, 2022
flip1995 pushed a commit to flip1995/rust that referenced this pull request Mar 24, 2022
[generator_interior] Be more precise with scopes of borrowed places

Previously the generator interior type checking analysis would use the nearest temporary scope as the scope of a borrowed value. This ends up being overly broad for cases such as:

```rust
fn status(_client_status: &Client) -> i16 {
    200
}

fn main() {
    let client = Client;
    let g = move || match status(&client) {
        _status => yield,
    };
    assert_send(g);
}
```

In this case, the borrow `&client` could be considered in scope for the entirety of the `match` expression, meaning it would be viewed as live across the `yield`, therefore making the generator not `Send`.

In most cases, we want to use the enclosing expression as the scope for a borrowed value which will be less than or equal to the nearest temporary scope. This PR changes the analysis to use the enclosing expression as the scope for most borrows, with the exception of borrowed RValues which are true temporary values that should have the temporary scope. There's one further exception where borrows of a copy such as happens in autoref cases also should be ignored despite being RValues.

Joint work with `@nikomatsakis`

Fixes rust-lang#57017

r? `@tmandry`
bors added a commit to rust-lang-ci/rust that referenced this pull request May 21, 2022
Drop Tracking: Implement `fake_read` callback

This PR updates drop tracking's use of `ExprUseVisitor` so that we treat `fake_read` events as borrows. Without doing this, we were not handling match expressions correctly, which showed up as a breakage in the `addassign-yield.rs` test. We did not previously notice this because we still had rather large temporary scopes that we held borrows for, which changed in rust-lang#94309.

This PR also includes a variant of the `addassign-yield.rs` test case to make sure we continue to have correct behavior here with drop tracking.

r? `@nikomatsakis`
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jun 4, 2022
Drop Tracking: Implement `fake_read` callback

This PR updates drop tracking's use of `ExprUseVisitor` so that we treat `fake_read` events as borrows. Without doing this, we were not handling match expressions correctly, which showed up as a breakage in the `addassign-yield.rs` test. We did not previously notice this because we still had rather large temporary scopes that we held borrows for, which changed in rust-lang#94309.

This PR also includes a variant of the `addassign-yield.rs` test case to make sure we continue to have correct behavior here with drop tracking.

r? `@nikomatsakis`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

async/await: awaiting inside a match block captures borrow too eagerly
7 participants